@rswarbrick @ziuziakowska

I haven't reviewed this carefully, but it seems kind of reasonable to me. Presumably, your motivation is to avoid needing the (slightly hacky) otbn-as Python script that I wrote for OpenTitan? One downside is that you end up requiring a particular toolchain if you want to assemble programs.

Is there a reason that it's worth paying this cost?

For us it would be nice to have access to the full gnu-as feature set for OTBN code. It would allow us
to write cryptographic code that is easier to write, read and maintain and could be made faster. Some
features that are not possible with otbn_as.py:

• Constants (+expressions) for all instructions.
• Macros are completely disabled.
  • In general, all the .* commands for all OTBN instructions.
• File inclusions.

For reference, ZeroRISC hacked together some partial constant and macro support in otbn_as.py that
allowed them to write a pretty concise ML-DSA implementation. It's very brittle, I think it is easier
to simply extend the binutils like in this PR.

That sounds sensible to me. I assume we're planning to add a dependency on this binutils and delete otbn_as once this lands? I REALLY don't want to have two different paths

@andrea-caforio what are you using for testing this? I assume we have a way to easily and (somewhat) exahaustively test this against otbn_as.py?

Regarding the broader PR, adding such support to the toolchain proper makes sense to me. I remember people occasionally struggling with some limitations of otbn_as.py, even before the recent motivations @andrea-caforio mentioned.

I REALLY don't want to have two different paths

If we have a way to easily test the two OTBN assemblers and fairly exhaustively test them, that doesn't sound too bad to me. I imagine it might be useful to keep the simple Python variant around for a while more, in case people need to test something with an older version of binutils, without this downstream OBTN support.

@andrea-caforio what are you using for testing this? I assume we have a way to easily and (somewhat) exahaustively test this against otbn_as.py?

Regarding the broader PR, adding such support to the toolchain proper makes sense to me. I remember people occasionally struggling with some limitations of otbn_as.py, even before the recent motivations @andrea-caforio mentioned.

I REALLY don't want to have two different paths

If we have a way to easily test the two OTBN assemblers and fairly exhaustively test them, that doesn't sound too bad to me. I imagine it might be useful to keep the simple Python variant around for a while more, in case people need to test something with an older version of binutils, without this downstream OBTN support.

Currently, I've been compiling existing OTBN apps with this and checking whether the generated binaries are
identical to what otbn_as.py produces. This could be automatized.

Another option would be to use the built-in test rig of gnu-as that allows for verifying generated binaries.
I have to check it out.

I haven't reviewed this carefully, but it seems kind of reasonable to me. Presumably, your motivation is to avoid needing the (slightly hacky) otbn-as Python script that I wrote for OpenTitan? One downside is that you end up requiring a particular toolchain if you want to assemble programs.

Is there a reason that it's worth paying this cost?

I haven't had to use/interact with OTBN so I do not know where the tooling used lives, however the plan for this repository is to be the new Binutils source for the Ibex toolchain (once we upgrade LLVM and do a new release featuring both of these), so this will make its way into the internal toolchain build that Opentitan pulls in.

I think + as an extension name might not be ideal, although we are stepping into weird territory here - as I understand the OTBN instruction set is more of a new "base" ISA that is incompatible with rv32i, etc, than an "extension" of those, though adding a new base ISA is probably too intrusive of a change to bother with. I would prefer the letter o for OTBN.

We should use the x suffix for vendor extensions. For instance, xlowriscotbn or lrotbn.

I'm in favor of adding proper OTBN support to the assembler as it helps a lot to simplify OTBN programming.

However, this extension will duplicate the instruction definition. As of now, the OTBN instructions are defined in yml files (hw/ip/otbn/data/bignum-insns.yml and hw/ip/otbn/data/base-insns.yml). These files are used for documentation as well as DV (the random instruction generator (RIG) generates the test programs based on these). When we add this extension, the instructions are defined in the extension as well. So we have to maintain two definitions and more importantly ensure that these definitions are in sync. This seems a little bit cumbersome and prone to errors.

Is there a smart way of ensuring this?

We should use the x suffix for vendor extensions. For instance, xlowriscotbn or lrotbn.

I think either of these is fine. I think the spec doesn't have provisions for a custom "base" ISA so we should treat it as a "extension" like this.

I'm in favor of adding proper OTBN support to the assembler as it helps a lot to simplify OTBN programming.

However, this extension will duplicate the instruction definition. As of now, the OTBN instructions are defined in yml files (hw/ip/otbn/data/bignum-insns.yml and hw/ip/otbn/data/base-insns.yml). These files are used for documentation as well as DV (the random instruction generator (RIG) generates the test programs based on these). When we add this extension, the instructions are defined in the extension as well. So we have to maintain two definitions and more importantly ensure that these definitions are in sync. This seems a little bit cumbersome and prone to errors.

Is there a smart way of ensuring this?

For the bitmanip work, I defined the instructions in a local copy of RISC-V opcodes, which uses the encoding definition to generate these mask and match constants in the format used by binutils, OpenSBI, etc. The syntax for those files looks like this:

bext     rd rs1 rs2 31..25=0b0000100 14..12=0b110 6..2=0b01100 1..0=3
bdep     rd rs1 rs2 31..25=0b0100100 14..12=0b110 6..2=0b01100 1..0=3

With the following output:

#define MATCH_BDEP 0x48006033
#define MASK_BDEP 0xfe00707f
#define MATCH_BEXT 0x8006033
#define MASK_BEXT 0xfe00707f

#ifdef DECLARE_INSN
DECLARE_INSN(bdep, MATCH_BDEP, MASK_BDEP)
DECLARE_INSN(bext, MATCH_BEXT, MASK_BEXT)
#endif

The yaml data for the instructions seems to look quite similar, and we could make some (maybe intrusive) changes to implement the OTBN operand types and use a script to map the yaml to the RISC-V opcodes format.

- mnemonic: bn.addc
  synopsis: Add with Carry
  operands: *bn-add-operands
  syntax: *bn-add-syntax
  doc: |
    Adds two WDR values and the Carry flag value, writes the result to the
    destination WDR, and updates the flags. The content of the second source
    WDR can be shifted by an unsigned immediate before it is consumed by the
    operation.
  errs: []
  iflow:
    - to: [wrd, flags-all]
      from: [wrs1, wrs2, flags-c]
  encoding:
    scheme: bnaf
    mapping:
      fg: flag_group
      shift_type: shift_type
      shift_bits: shift_bits
      wrs2: wrs2
      wrs1: wrs1
      funct3: b010
      wrd: wrd

Thank @rswarbrick @luismarques @ziuziakowska @etterli for the reviews. I really
appreciate it. I'm not sure when I get around to implementing it because PQC is
a more pressing task for me currently.

In any case, I will try to come up with an appropriate testing framework to properly
verify the correctness of the patch.